Check if merge is aborted during HNSW graph construction#16368
Check if merge is aborted during HNSW graph construction#16368jeho-rpls wants to merge 5 commits into
Conversation
Graph construction is a pure-CPU loop that performs no writes, so it never observed OneMerge's abort flag: IndexWriter#rollback/abortMerges blocked until the entire graph was built (tens of minutes for large vector segments). Wire MergeState#checkAborted from Lucene99HnswVectorsWriter through the HNSW graph mergers into the builders, and invoke it before every node insertion in HnswGraphBuilder#addGraphNodeInternal - the single point that all insertion paths (full rebuild, graph-join, concurrent workers) funnel through. Flush-time graph builds are unaffected (no check set).
|
The CI failure is unrelated to this change: fails deterministically with |
jeho-rpls
force-pushed
the
hnsw-merge-abort-16367
branch
from
5e9919a to
f7608f1
Compare
|
Opened #16369 with a fix for the unrelated test failure above. |
Add a variant with numMergeWorkers=2 so the abort check forwarding in HnswConcurrentMergeBuilder is exercised, and relax the rollback bound from 5s to 10s for slow CI machines. Without the fix the three variants block rollback for 27s (full rebuild), 31s (graph join) and 14s (concurrent) on this index.
# Conflicts: # lucene/CHANGES.txt
|
Is this really needed? When do we ever call abortMerges()? It looks to me as if it gets called when an exception happens while closing an IndexWriter, or explicitly rolling back. Seem like kind of exceptional cases, although maybe we would want rollbacks to be faster? Do other merge operations check for aborted status? |
|
Hi @msokolov, thanks for taking a look!
That is how we hit this.
Elasticsearch reports hitting a similar issue in production and has an open downstream workaround (elastic/elasticsearch#152342), but it lives in their stateless plugin only, and the workaround's author notes it is partial coverage. A check in the graph builder covers every directory implementation.
Yes, exactly. Faster rollback is the whole motivation. The wait also buys nothing. The writer is rolling back, so the graph being built is discarded the moment it completes.
They do.
Graph construction is the odd one out. It performs no writes and no checksum reads for tens of minutes, so it never observes the flag. In our profiling it was 93.6% of the merge CPU, while the checksum phase covered by #16281 was 0.1%. |
2 participants
Fixes #16367.
HNSW graph construction never checked
OneMerge's abort flag. It is a pure-CPU loop that performs no writes, soIndexWriter#rollbackandabortMerges()blocked until the entire graph was built. For production-sized vector segments that means tens of minutes (measurements in the issue).Changes
HnswBuildergainssetAbortCheck(IORunnable), andHnswGraphBuilderinvokes it before every node insertion inaddGraphNodeInternal.addVectors, graph join viaMergingHnswGraphBuilder, andHnswConcurrentMergeBuilderworkers (which forward the check).Lucene99HnswVectorsWriterpassesmergeState::checkAbortedinto the graph mergers (new constructor overloads, existing constructors unchanged).checkIntegrity(OneMerge)from Check if merge is aborted during file integrity checksums #16281.Testing
TestHnswMergeAbortexercises all three merge paths end-to-end (rollback during aforceMergeof a 48k x 96d index). Without the fix, rollback blocks until the build finishes. With the fix, it returns in milliseconds.MergingHnswGraphBuilder): blocked 31 snumMergeWorkers = 2,HnswConcurrentMergeBuilder): blocked 14 s./gradlew tidyand:lucene:core:checkpass.